aio: add h1 title to floating table of contents #16959

petebacondarwin · 2017-05-23T12:50:44Z

petebacondarwin · 2017-05-23T12:53:42Z

aio/src/app/embedded/toc/toc.component.ts

-
-      Observable.combineLatest(this.tocService.activeItemIndex, this.items.changes.startWith(this.items))
+      // We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes,
+      // which are, in turn, triggered rendering that happened due to a ChangeDetection.


petebacondarwin · 2017-05-24T08:01:21Z

I rebased, fixed the conflict, and broke up the big commit into smaller ones to make the refactoring more easy to follow.

mary-poppins · 2017-05-24T08:10:49Z

The angular.io preview for a4fecef is available here.

mary-poppins · 2017-05-24T08:19:00Z

The angular.io preview for c496a15 is available here.

gkalpak

A few nit-picks. Nothing blocking.
Loved the refactorings ❤️

gkalpak · 2017-05-24T14:12:56Z

aio/src/app/embedded/toc/toc.component.ts

+      // We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes,
+      // which, in turn, are caused by the rendering that happened due to a ChangeDetection.
+      // Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular.
+      Observable.combineLatest(this.tocService.activeItemIndex.subscribeOn(asap), this.items.changes.startWith(this.items))


gkalpak · 2017-05-24T14:24:59Z

aio/src/app/embedded/toc/toc.component.ts

 import 'rxjs/add/operator/takeUntil';

 import { ScrollService } from 'app/shared/scroll.service';
 import { TocItem, TocService } from 'app/shared/toc.service';

+type TocType = 'None' |'Floating' |'EmbeddedSimple' |'EmbeddedExpandable';


No whitespace after |? 😞

gkalpak · 2017-05-24T14:25:42Z

aio/src/app/embedded/toc/toc.component.html

-<div *ngIf="hasToc" [class.collapsed]="isCollapsed">
-  <button *ngIf="!isEmbedded" type="button" class="toc-heading"
-    (click)="toTop()" title="Top of page">Contents</button>
+<div *ngIf="type != 'None'" class="toc-inner" [class.collapsed]="isCollapsed">


OOC, why != vs !==?

gkalpak · 2017-05-24T14:27:20Z

aio/src/app/embedded/toc/toc.component.ts

+                            'EmbeddedExpandable' :
+                          'EmbeddedSimple' :
+                        'Floating' :
+                      'None';


Ternary operator hell 😁

Nah! It's a pyramid of beauty!

Just to be clear: I like it 😃

gkalpak · 2017-05-24T14:39:20Z

aio/src/styles/2-modules/_toc.scss

@@ -67,7 +56,8 @@ aio-toc > div {
  }

  button.toc-heading,
-  div.toc-heading {
+  div.toc-heading,
+  ul.toc-list li.h1 {


I wonder if we need the element specifiers at all. Why not just .toc-heading, .toc-list .h1 { ... }?

That is probably correct. I was just sort of following the convention there already.

But I think that Stef will change some of this anyway

gkalpak · 2017-05-24T14:44:23Z

aio/src/styles/2-modules/_toc.scss

@@ -134,7 +124,7 @@ aio-toc > div {
    transition: all 0.3 ease-in-out;


Not directly related to this PR, but I noticed this value is invalid and causes the style to be ignored. It should be 0.3s (the s is currently missing).

I'll sneak that in.

gkalpak · 2017-05-24T14:45:04Z

aio/src/styles/2-modules/_toc.scss

+    display: block;
+    height: 1px;
+    width: 40%;
+    margin: 24px 0px 10px;


24px feels too much. margin: 10px 0 looks better imo 😃

I think this is due to change in Stef's PR.

gkalpak · 2017-05-24T14:49:26Z

aio/src/app/embedded/toc/toc.component.ts

-              tocList[i].isSecondary = true;
-            }
-          }
+          const count = this.tocList.reduce((c, item) => item.level !== 'h1' ? c + 1 : c, 0);


Nit: this.tocList.filter(item => item.level !== 'h1').length is slightly more readable 😳

but slower and uses more memory :-P

I know, I know 😞 But Igor said to opt for readability over performance 😛

gkalpak · 2017-05-24T15:00:00Z

aio/src/app/embedded/toc/toc.component.html

-      <a [href]="toc.href" [innerHTML]="toc.content"></a>
-    </li>
+    <ng-container *ngFor="let toc of tocList; let i = index">
+      <li #tocItem title="{{toc.title}}" *ngIf="type == 'Floating' || toc.level != 'h1'"


It might make sense to avoid this ngIf, by separating the h1 element from the rest (in the component).

I thought about that but it just moves the complexity somewhere else.
Not displaying the h1 IMO is a visual choice, hence the responsibility of the template.

The decision about dislaying it or not will still be in the template. But the template won't have to "extract" it from the tocList (which sounds rather "business-logic-y").
_{I don't feel at all strognly about it, though.}

I think that then you would need complexity in both the component class (to extract the h1) and then in the template (where it would need to duplicate the li element for the h1).

¯\_(ツ)_/¯

Actually also it become even more complex when you then try to get the activeIndex to line up. This way, it just works :-)

gkalpak · 2017-05-24T15:02:38Z

aio/src/app/embedded/toc/toc.component.spec.ts

      fixture.detectChanges();
      expect(tocComponentDe.queryAllNodes(By.css('li')).length).toBe(3);
    });

+    it('should only display H2 and H3 TocItems', () => {
+      tocService.tocList.next([tocItem('Heading A', 'h1'), tocItem('Heading B'), tocItem('Heading C')]);


No test for H3 😟

petebacondarwin · 2017-05-24T16:15:59Z

@gkalpak Pushed a bunch of fixes from your feedback, some of which will be squashed. PTAL

mary-poppins · 2017-05-24T16:25:14Z

The angular.io preview for 4bb3bc7 is available here.

gkalpak

Still LGTM

gkalpak · 2017-05-24T16:20:25Z

aio/src/app/embedded/toc/toc.component.html


-  <button *ngIf="hasSecondary" type="button" class="toc-heading embedded secondary"
-    (click)="toggle(false)"
+  <button *ngIf="type == 'EmbeddedExpandable'" type="button" (click)="toggle(false)"


There's one more here 😛

gkalpak · 2017-05-24T16:20:48Z

aio/src/app/embedded/toc/toc.component.ts

 import 'rxjs/add/operator/takeUntil';

 import { ScrollService } from 'app/shared/scroll.service';
 import { TocItem, TocService } from 'app/shared/toc.service';

+type TocType = 'None' |'Floating' |'EmbeddedSimple' | 'EmbeddedExpandable';


There are more missing whitespaces here.

gkalpak · 2017-05-24T16:25:23Z

aio/src/app/embedded/toc/toc.component.ts

@@ -92,3 +92,7 @@ export class TocComponent implements OnInit, AfterViewInit, OnDestroy {
    this.scrollService.scrollToTop();
  }
 }
+
+function count<T>(array: T[], fn: (T) => boolean) {
+  return array.reduce((count, item) => fn(item) ? count + 1 : count, 0);


return array.filter(fn).length is more readable 😛

This makes the styling less fragile to changes in the HTML

We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular.

Closes angular#16900

mary-poppins · 2017-05-24T18:01:33Z

The angular.io preview for c9f93b9 is available here.

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

There seems to have been a bad rebase of #16228 on top of #16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes #17098

* refactor(aio): use explicit CSS class for TOC container This makes the styling less fragile to changes in the HTML * fix(aio): schedule TocComponent.activeIndex updates via AsapScheduler We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular. * refactor(aio): do not instantiate floating ToC if not displayed * feat(aio): display the h1 at the top of the floating TOC Closes angular#16900 * refactor(aio): combine the TOC booleans flags into a "type" state * refactor(aio): remove unnecessary `hostElement` property * fix(aio): ensure that transition works on TOC * fix(aio): use strict equality in ToC template

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

* refactor(aio): use explicit CSS class for TOC container This makes the styling less fragile to changes in the HTML * fix(aio): schedule TocComponent.activeIndex updates via AsapScheduler We use the `asap` scheduler because updates to `activeItemIndex` are triggered by DOM changes, which, in turn, are caused by the rendering that happened due to a ChangeDetection. Without asap, we would be updating the model while still in a ChangeDetection handler, which is disallowed by Angular. * refactor(aio): do not instantiate floating ToC if not displayed * feat(aio): display the h1 at the top of the floating TOC Closes angular#16900 * refactor(aio): combine the TOC booleans flags into a "type" state * refactor(aio): remove unnecessary `hostElement` property * fix(aio): ensure that transition works on TOC * fix(aio): use strict equality in ToC template

There seems to have been a bad rebase of angular#16228 on top of angular#16959, which affected ToC styles from both PRs. This commit restores the horizontal line under `.h1` elements and the vertical blue bar on the left-hand side of the ToC (with the circle running along the bar to indicate the active section). Fixes angular#17098

angular-automatic-lock-bot · 2019-09-11T16:20:41Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer labels May 23, 2017

googlebot added the cla: yes label May 23, 2017

petebacondarwin added this to REVIEW in docs-infra May 23, 2017

petebacondarwin commented May 23, 2017

View reviewed changes

petebacondarwin force-pushed the aio-toc-floating-title branch from a6c01b3 to a4fecef Compare May 24, 2017 08:00

petebacondarwin force-pushed the aio-toc-floating-title branch from a4fecef to c496a15 Compare May 24, 2017 08:09

gkalpak approved these changes May 24, 2017

View reviewed changes

petebacondarwin added 8 commits May 24, 2017 18:35

refactor(aio): use explicit CSS class for TOC container

42d75bd

This makes the styling less fragile to changes in the HTML

refactor(aio): do not instantiate floating ToC if not displayed

2035c56

feat(aio): display the h1 at the top of the floating TOC

84d40a7

Closes angular#16900

refactor(aio): combine the TOC booleans flags into a "type" state

6727a3f

refactor(aio): remove unnecessary hostElement property

61125b6

fix(aio): use strict equality in ToC template

42a3bf4

fix(aio): ensure that transition works on TOC

c9f93b9

petebacondarwin force-pushed the aio-toc-floating-title branch from 4bb3bc7 to c9f93b9 Compare May 24, 2017 17:51

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 24, 2017

petebacondarwin moved this from REVIEW to MERGE in docs-infra May 24, 2017

chuckjaz merged commit 966eb2f into angular:master May 24, 2017

petebacondarwin removed this from MERGE in docs-infra May 25, 2017

petebacondarwin deleted the aio-toc-floating-title branch May 25, 2017 10:05

gkalpak mentioned this pull request May 30, 2017

fix(aio): fix ToC styling #17110

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

		@@ -134,7 +124,7 @@ aio-toc > div {
		transition: all 0.3 ease-in-out;

aio: add h1 title to floating table of contents #16959

aio: add h1 title to floating table of contents #16959

Conversation

petebacondarwin commented May 23, 2017

Choose a reason for hiding this comment

petebacondarwin commented May 24, 2017

mary-poppins commented May 24, 2017

mary-poppins commented May 24, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak May 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented May 24, 2017

mary-poppins commented May 24, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented May 24, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

gkalpak May 24, 2017 •

edited